-
Notifications
You must be signed in to change notification settings - Fork 156
Unequip Items/Masks from C-Buttons #1429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
mckinlee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on this. I did not experience any issues in my testing. Codewise you are looking good, but the new VBs in GameInteractor_VanillaBehavior.h need to be moved above VB_KALEIDO_UNPAUSE_CLOSE to maintain alphabetical order.
done |
Eblo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works nicely. My concerns are primarily with code flow and some tidying up.
|
|
||
| void RegisterDpadPageSwitchPrevention() { | ||
| COND_VB_SHOULD(VB_KALEIDO_SWITCH_PAGE_WITH_DPAD, CVarGetInteger("gEnhancements.Dpad.DpadEquips", 0), { | ||
| PlayState* play = va_arg(args, PlayState*); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The globally available gPlayState should give you what you need instead of having to pass a PlayState* arg, or is there a reason you had to do it this way?
|
|
||
| void RegisterItemUnequip() { | ||
| COND_VB_SHOULD(VB_KALEIDO_EQUIP_ITEM_TO_BUTTON, CVAR, { | ||
| PlayState* play = va_arg(args, PlayState*); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
|
|
||
| } else if (pauseCtx->equipTargetCBtn == PAUSE_EQUIP_D_DOWN) { | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } else if (pauseCtx->equipTargetCBtn == PAUSE_EQUIP_D_DOWN) { | |
| } else if (pauseCtx->equipTargetCBtn == PAUSE_EQUIP_D_DOWN) { |
| DPAD_SLOT_EQUIP(0, EQUIP_SLOT_D_DOWN) = pauseCtx->equipTargetSlot; | ||
| Interface_Dpad_LoadItemIconImpl(play, EQUIP_SLOT_D_DOWN); | ||
| } else if (pauseCtx->equipTargetCBtn == PAUSE_EQUIP_D_UP) { | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | ||
| // #endregion | ||
|
|
||
| // Item unequip enhancement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Item unequip enhancement |
| DPAD_SLOT_EQUIP(0, EQUIP_SLOT_D_DOWN) = pauseCtx->equipTargetSlot; | ||
| Interface_Dpad_LoadItemIconImpl(play, EQUIP_SLOT_D_DOWN); | ||
| } else if (pauseCtx->equipTargetCBtn == PAUSE_EQUIP_D_UP) { | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return; | ||
| } | ||
| pauseCtx->equipTargetCBtn = PAUSE_EQUIP_D_UP; | ||
| else if (CVarGetInteger("gEnhancements.Dpad.DpadEquips", 0) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having trouble understanding why if (CVarGetInteger("gEnhancements.Dpad.DpadEquips", 0) was moved from being one overarching condition with further nested conditions, to being an AND condition for every case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is from when I was troubleshooting to try and understand why the enhancement didn't wanna work with the dpad and I thought I removed it but ig not????
| // #endregion | ||
|
|
||
| // #region 2S2H [Enhancement] | ||
| // Item unequip enhancement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment tag is mostly for inline changes to source code. VB names are self-explanatory, plus this entire DPad input region already labeled as such.
| // #endregion | |
| // #region 2S2H [Enhancement] | |
| // Item unequip enhancement |
| #define CVAR CVarGetInteger(CVAR_NAME, 0) | ||
|
|
||
| void RegisterDpadPageSwitchPrevention() { | ||
| COND_VB_SHOULD(VB_KALEIDO_SWITCH_PAGE_WITH_DPAD, CVarGetInteger("gEnhancements.Dpad.DpadEquips", 0), { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you go ahead and follow the same pattern you did with ItemUnequip? Just so we're not repeating one name and not the other. CVAR_DPAD_EQUIPS is probably as good a name as any.
improved code flow in general and undid unnecessary edit to dpad equips
|
something broke, will check in a bit |
|
found what it was, just didn't update the GameInteractor_Should calls |
|
done |
mckinlee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on changes that addressed Eblo's feedback I have one small change request.
mckinlee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did some sanity testing. Still works like a charm. I did notice a potential UX concern, but I don't think that's blocking. If a magic arrow is equipped, you cannot unequip it on the ITEM_BOW slot. It requires using the applicable magic item slot only. If you choose to address that, I'll re-review, but I'm approving as is.
|
do you mean to be able to unequip any bow + magic arrow from the bow item? I didn't even think about that possibility tbh... would it be preferable to have it work like that? |
You know how when you have a item equipped to C/D it draws that white border around the item in the pause menu as a indicator? When you equip a magic arrow, that border is drawn over the ITEM_BOW slot, not specifically the magic arrow slot thats equipped. So my instinct as a player was to try to unequip it using that particular slot. I had gotten use to seeing the white border as a indicator for what can be unequipped for all other items. I hope that makes sense. I'd love to hear other opinions on this, but my first thought was to keep what you have as-is, but additionally allow unequipping the magic arrow with the regular bow slot. |
|
that does make sense tbh |
|
ok... I think that's everything |
mckinlee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! LGTM.
Eblo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking cleaner now! One final nitpick, but if you can address my two standing cleanup feedback suggestions, I think this will be good to go. Mostly just to reduce the source footprint a little more.
mm/src/overlays/kaleido_scope/ovl_kaleido_scope/z_kaleido_item.c
Outdated
Show resolved
Hide resolved
* Bg_Danpei_Movebg OK * Cleanup * PR comments * Couple small things * Address the nit * Flags --------- Co-authored-by: Derek Hensley <hensley.derek58@gmail.com>
null
Build Artifacts